feat: terminateVisibilityTimeout = number to customise the timeout #520
Conversation
|
CLA Assistant Lite bot CLA CHECK All Contributors have signed the CLA |
|
I have read the CLA Document and I hereby sign the CLA |
Frederick888
force-pushed
the
custom-vis-timeout-on-err
branch
from
524d1cf to
a73942f
Compare
src/consumer.ts
Outdated
| if (this.terminateVisibilityTimeout) { | ||
| await this.changeVisibilityTimeout(message, 0); | ||
| if (this.terminateVisibilityTimeout !== false) { | ||
| const timeout = this.terminateVisibilityTimeout === true ? 0 : this.terminateVisibilityTimeout |
There was a problem hiding this comment.
Thanks for the PR, this makes sense. I am currently thinking if it makes sense to add a new option though (terminateVisibilityTimeoutMs) rather than this logic and then just default that to 0 and allow it to be changed.
There was a problem hiding this comment.
Thanks for the review! To be honest I'm on the fence too. If we reuse the terminateVisibilityTimeout option, the naming is a bit confusing; if we add a terminateVisibilityTimeoutMs, without looking at their types, there will appear to be a timeout and a timeout in milliseconds at the same time, and I'm not sure what should happen if terminateVisibilityTimeout == false but terminateVisibilityTimeoutMs is set.
I'm happy to leave the decision to you. I can update the code and docs if you opt for a new option in the end.
PS: Forgot to npm run format. Just pushed :)
There was a problem hiding this comment.
Yeah that makes sense too, I think I'll just merge this into canary for now and think on it, if I don't decide against it by Wednesday, I'll push it up to main.
Thanks a lot for the contribution!
This allows users to introduce a delay before the message can be received again. In some cases this can improve the overall success rate.
Frederick888
force-pushed
the
custom-vis-timeout-on-err
branch
from
a73942f to
092c3b9
Compare
Resolves #523
Description:
This allows users to introduce a delay before the message can be received again. In some cases this can improve the overall success rate.
Type of change:
Why is this change required?:
When tasks are not time critical, a small delay can mitigate some intermittent errors, or give supervisors like K8s a little time to kill broken consumers.
Code changes:
terminateVisibilityTimeoutoption has been updated to accept custom visibility timeouts.I reused the option as I thought it was the most non-intrusive way to introduce this feature. If it appears to be confusing, I can also add a new option. But since the underlying SQS command is the same, I think it's ok.
Checklist: